Skip to content

fix(enrichment): join adjacent added lines before secret-scan matching#2468

Merged
JSONbored merged 1 commit into
mainfrom
fix/secret-scan-cross-line-2454
Jul 2, 2026
Merged

fix(enrichment): join adjacent added lines before secret-scan matching#2468
JSONbored merged 1 commit into
mainfrom
fix/secret-scan-cross-line-2454

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • scanPatch (the secret-scan analyzer's only live entry point, via scanSecrets/secret/descriptor.ts) matched each added diff line independently, so a real credential split across two added lines via string concatenation (const a = "AKIA..."; const b = a + "REST";) never appeared as a single matchable token on either line and evaded every RULES regex.
  • Extracts quoted string-literal contents per added line and, when a line doesn't already match on its own, joins the immediately-preceding line's last literal with the current line's first literal and tests that too — downgraded to medium confidence since a joined pair is a heuristic, not a direct contiguous match.
  • Bounded to consecutive added lines only: a hunk boundary, a context line, or a removed line resets the join window, so this only ever catches the realistic "two sequential variable assignments" shape, not an unbounded cross-file join.
  • Existing per-line detection is untouched and unweakened.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked an issue, or this is small enough that the summary explains why an issue is not needed.

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck (root — unaffected, no src/** changed)
  • npm run test:coverage — N/A for this PR's own diff: review-enrichment/** is a separate workspace excluded from the Codecov src/** patch gate (own npm run rees:test runner instead, see below)
  • npm run test:workers
  • npm run build:mcp
  • npm run test:mcp-pack
  • npm run ui:openapi:check
  • npm run ui:lint
  • npm run ui:typecheck
  • npm run ui:build
  • npm audit --audit-level=moderate — 0 vulnerabilities
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries

Also ran npm run rees:test (the analyzer package's own gate, part of test:ci): 374/374 passing, including a new review-enrichment/test/secret-scan.test.ts (9 tests) covering the pre-existing single-line rules plus 5 new tests for the cross-line join: the concatenation-split regression case itself, a context-line break, a hunk-boundary break, no-double-report when a line already matched directly, and a false-positive guard for two unrelated short literals. npm run db:migrations:check also unaffected/green.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed. Test fixtures build fake secret-shaped strings from split fragments at runtime rather than as contiguous literals, specifically so the fixture file itself doesn't trip GitHub push protection the way a real leaked credential would.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes — N/A, this is an enrichment analyzer, not an auth path.
  • API/OpenAPI/MCP behavior is updated and tested where needed. No OpenAPI/MCP surface changed.
  • UI changes — N/A, no UI changed.
  • Visible UI changes — N/A, no UI changed.
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs. No changelog edit.

Notes

  • Found via an adversarial audit of the review engine's security-relevant analyzers (which gate what counts as risky code in a PR). scanAddedLinesForSecrets (a second, unused export in the same file with the same per-line-only gap) was left untouched — it has zero callers anywhere in the codebase, so fixing it would add untestable dead-code coverage; out of scope for this fix.

Secret-scan matched each added diff line independently, so a real
credential split across two added lines via string concatenation (e.g.
`const a = "AKIA..."; const b = a + "REST";`) never appeared as a
single matchable token on either line and evaded every RULES regex.

Extract quoted string-literal contents per added line and, when the
line doesn't already match on its own, join the immediately-preceding
line's last literal with the current line's first literal and test
that too (downgraded to medium confidence — a joined pair is a
heuristic, not a direct match). Bounded to consecutive added lines
only: a hunk boundary, context line, or removed line resets the
window, so this only ever catches the realistic "two sequential
variable assignments" shape, not an unbounded cross-file join.

Existing per-line detection is untouched and unweakened. Added
review-enrichment/test/secret-scan.test.ts (previously untested)
covering both the pre-existing single-line rules and the new
cross-line join, including negative cases for context-line/hunk-
boundary breaks and duplicate-finding avoidance.

Fixes #2454
@dosubot dosubot Bot added the size:M label Jul 2, 2026
@loopover-orb

loopover-orb Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-02 05:12:38 UTC

2 files · 1 AI reviewer · 1 blocker · readiness 86/100 · CI pending · blocked

⏸️ Suggested Action - Manual Review

  • Possible leaked secret in the diff (private_key_block) — Remove the secret from the diff, rotate the exposed credential, then re-run the gate.

Review summary
The change extends `scanPatch` to catch a credential-shaped token split across two adjacent added string literals while preserving the existing direct per-line scan and hunk/context reset behavior. The implementation is small and the tests cover the main reachable paths: direct matches, adjacent joins, context gaps, hunk boundaries, and duplicate suppression. I do not see a must-fix correctness defect in the provided post-change files.

Nits — 5 non-blocking
  • nit: `review-enrichment/src/analyzers/secret-scan.ts:53` says the example is a runtime join, but the implementation only joins adjacent literal text and does not verify that the source lines actually concatenate those literals, so the comment should be tightened or the predicate should check for a `+` relationship.
  • nit: `review-enrichment/src/analyzers/secret-scan.ts:58` extracts raw escaped literal contents without unescaping, so a split value using escape sequences will not be reconstructed the way JavaScript evaluates it; that is acceptable for the current heuristic but worth documenting.
  • nit: `review-enrichment/test/secret-scan.test.ts:62` exercises a context line before two adjacent added lines and then a real context gap in the same test, which makes the test name less direct than the behavior under assertion.
  • In `review-enrichment/src/analyzers/secret-scan.ts:92`, either require the current/previous lines to look like string concatenation before joining or update the comment to state this is an adjacent-literal heuristic rather than a runtime-concatenation detector.
  • In `review-enrichment/test/secret-scan.test.ts:62`, split the pre-context sanity check from the actual gap assertion so the regression test reads as one behavior per test.

Concerns raised — review before merging

  • Possible leaked secret in the diff (private_key_block) — Remove the secret from the diff, rotate the exposed credential, then re-run the gate.
Signal Result Evidence
Code review ❌ 1 blocker 1 reviewer
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 65 registered-repo PR(s), 55 merged, 554 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 65 PR(s), 554 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: not available
  • Official Gittensor activity: 65 PR(s), 554 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • Triage stale or unlinked PRs.
  • No action.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@loopover-orb loopover-orb Bot added gittensor gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. labels Jul 2, 2026
@JSONbored JSONbored self-assigned this Jul 2, 2026
@JSONbored

Copy link
Copy Markdown
Owner Author

Secret key flag is a false positive, merging.

@JSONbored
JSONbored merged commit 8f2e0fb into main Jul 2, 2026
10 checks passed
@JSONbored
JSONbored deleted the fix/secret-scan-cross-line-2454 branch July 2, 2026 05:36
@github-project-automation github-project-automation Bot moved this from Todo to Done in gittensory - v1 roadmap Jul 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

1 participant